feat(android-distribution): Add module foundation with compilation stubs #4712
Conversation
runningcode
requested review from
adinauer,
romtsn,
stefanosiano,
markushi and
lcian
as code owners
|
| * Provides functionality to check for app updates and download new versions from Sentry's preprod | ||
| * artifacts system. | ||
| */ | ||
| public object Distribution { |
There was a problem hiding this comment.
This is almost exactly the same API as in the Emerge Build Distribution.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| public fun downloadUpdate(context: Context, info: UpdateInfo) { | ||
| val browserIntent = Intent(Intent.ACTION_VIEW, Uri.parse(info.downloadUrl)) | ||
| browserIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) | ||
| context.startActivity(browserIntent) | ||
| } |
There was a problem hiding this comment.
Potential bug: The downloadUpdate method calls startActivity without handling a potential ActivityNotFoundException, which can crash the app if no browser is installed.
-
Description: The
downloadUpdatemethod, part of the public SDK API, initiates a download by callingstartActivitywith anACTION_VIEWintent. However, it lacks error handling forActivityNotFoundException. This exception is thrown if no application on the device can handle the HTTP/HTTPS URL, such as when no web browser is installed. Because this is an unhandled exception in a public SDK method, it will propagate up and crash the host application. -
Suggested fix: Wrap the
startActivity(intent)call in atry-catchblock to handleActivityNotFoundException. Alternatively, check if an activity can handle the intent by callingcontext.packageManager.resolveActivity(intent, 0)before callingstartActivity.
severity: 0.6, confidence: 0.9
Did we get this right? 👍 / 👎 to inform future reviews.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| 85d7417 | 347.21 ms | 394.35 ms | 47.15 ms |
| b750b96 | 408.98 ms | 480.32 ms | 71.34 ms |
| ee747ae | 358.21 ms | 389.41 ms | 31.20 ms |
| ee747ae | 415.92 ms | 470.15 ms | 54.23 ms |
| b750b96 | 421.25 ms | 444.09 ms | 22.84 ms |
| c8125f3 | 383.82 ms | 441.66 ms | 57.84 ms |
| ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 85d7417 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.19 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
| c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
Previous results on branch: no/distribution-module-foundation
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| de2fe17 | 388.06 ms | 432.06 ms | 44.00 ms |
| c6cf0b8 | 366.29 ms | 420.61 ms | 54.32 ms |
| a18e682 | 395.94 ms | 443.64 ms | 47.70 ms |
| 3ea8703 | 376.34 ms | 437.15 ms | 60.81 ms |
| 0ed1688 | 397.64 ms | 447.40 ms | 49.76 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| de2fe17 | 1.58 MiB | 2.10 MiB | 532.30 KiB |
| c6cf0b8 | 1.58 MiB | 2.10 MiB | 532.37 KiB |
| a18e682 | 1.58 MiB | 2.10 MiB | 532.50 KiB |
| 3ea8703 | 1.58 MiB | 2.10 MiB | 532.37 KiB |
| 0ed1688 | 1.58 MiB | 2.10 MiB | 532.30 KiB |
| <application> | ||
| <provider | ||
| android:name="io.sentry.android.distribution.DistributionContentProvider" | ||
| android:authorities="${applicationId}.sentry-distribution-provider" |
There was a problem hiding this comment.
Looks like we can make this a bit shorter:
- Just
.SentryDistributionProviderfor name ${applicationId}.SentryDistributionProviderfor android:authorities to match prtg
sentry-android-distribution/src/main/java/io/sentry/android/distribution/Distribution.kt
Outdated
Show resolved
Hide resolved
| * the Distribution SDK is available without requiring manual initialization in | ||
| * Application.onCreate(). | ||
| */ | ||
| public class DistributionContentProvider : ContentProvider() { |
There was a problem hiding this comment.
It might be worth having a look at EmptySecureContentProvider, I remember we had some security issue reports around default content providers.
| * @param sentryBaseUrl Base URL for Sentry API (defaults to https://sentry.io) | ||
| * @param buildConfiguration Optional build configuration name for filtering | ||
| */ | ||
| public data class DistributionOptions( |
There was a problem hiding this comment.
It's kinda discouraged (e.g. see this article) to use data classes for public APIs, I'm not sure how often this changes - but maybe makes sense to not use it here too.
sentry-android-distribution/src/main/java/io/sentry/android/distribution/Distribution.kt
Outdated
Show resolved
Hide resolved
sentry-android-distribution/src/main/java/io/sentry/android/distribution/Distribution.kt
Outdated
Show resolved
Hide resolved
sentry-android-distribution/src/main/java/io/sentry/android/distribution/Distribution.kt
Outdated
Show resolved
Hide resolved
| * @param context Android context | ||
| * @return UpdateStatus indicating if an update is available, up to date, or error | ||
| */ | ||
| public fun checkForUpdate(context: Context): UpdateStatus { |
There was a problem hiding this comment.
Right now triggering the update check and getting the latest update status is one operation. I'm wondering what the ideal scenario would look like, and if it could make sense to split those two.
- If the ContentProvider would trigger an update check automatically during init, it could make sense to have a
registerUpdateListener()API, which emits the latest status (if any) + and future triggered checks - If you expect people to add "check for update" buttons to their app the existing APIs would better suit this
There was a problem hiding this comment.
This is a good point. We expect that most people will not use the API directly, it will just work automatically in the background. When an update is available, then it will be displayed to the user directly.
The API is for scenarios where users want more control over how the update check works.
| val organizationSlug: String, | ||
| val projectSlug: String, | ||
| val sentryBaseUrl: String = "https://sentry.io", | ||
| val buildConfiguration: String? = null, |
There was a problem hiding this comment.
What could be an example value for this?
There was a problem hiding this comment.
buildConfiguration is like debug or release. You can only check for updates with the same build configuration. You can’t be on a debug build and pull a release update.
| * Provides functionality to check for app updates and download new versions from Sentry's preprod | ||
| * artifacts system. | ||
| */ | ||
| public object Distribution { |
There was a problem hiding this comment.
For new-ish features like logging and replay, we're exposing top level entry points under the Sentry class. E.g. Sentry.logger() and Sentry.replay(). It could make sense to do the same for distribution as well.
There was a problem hiding this comment.
This is a good point, I looked in to it and it seems like a bigger change, I will add this in a future PR.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This PR establishes the foundational structure for the sentry-android-distribution module with compilation stubs that enable parallel development of individual components.
### Changes
- Android module configuration with necessary dependencies
- AndroidManifest.xml with ContentProvider for auto-initialization
- Distribution object with init(), isEnabled(), checkForUpdate() methods
- DistributionOptions data class for configuration
- UpdateStatus sealed class for result types
- UpdateInfo data class for update details
- Internal stub implementations that compile successfully
### Implementation Strategy
- All methods return placeholder errors ("Implementation coming in future PR")
- Follows zero-dependency design (only depends on sentry module)
- Enables parallel development of binary identifier, HTTP client, API models, and core logic
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
- Fix ActivityNotFoundException in downloadUpdate method - Update AndroidManifest provider to use shorter naming convention - Add EmptySecureContentProvider for security - Convert DistributionOptions from data class to regular class - Add initOrder comment explaining initialization sequence 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add lambda-based init pattern matching SentryAndroid.init - Rename checkForUpdate to checkForUpdateBlocking for clarity - Replace CompletableFuture with simple callback approach - Convert DistributionOptions to mutable builder pattern - Add example for buildConfiguration parameter 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add automatic distribution module detection in SentryAndroid.java - Create DistributionIntegration for seamless auto-enablement when module present - Add Sentry.distribution() top-level API using reflection for build-time safety - Remove ContentProvider approach in favor of Integration pattern - Update Distribution API to use callback-based async methods - Fix ActivityNotFoundException handling in downloadUpdate method Follows existing patterns from replay/timber/fragment integrations for consistency. Module works automatically when included, provides compile errors when not. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
runningcode
force-pushed
the
no/distribution-module-foundation
branch
from
c1d435c to
b68cb00
Compare
…irect instantiation - Remove sentry-android-core dependency from distribution module (only needs sentry module) - Add distribution as compileOnly dependency in sentry-android-core - Use direct DistributionIntegration instantiation instead of reflection - Eliminates circular dependency and follows same pattern as other integrations The distribution module only needs Integration/IScopes/SentryOptions from core sentry, not anything from sentry-android-core, making the architecture cleaner. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
No longer needed since this is a single PR implementation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
| * | ||
| * @return The distribution API object that provides update checking functionality | ||
| */ | ||
| public static @Nullable Object distribution() { |
There was a problem hiding this comment.
This method uses reflection whereas the replay() and logger() methods work by calling no-op APIs when the classes aren’t available. To save the extra boilerplate of creating the interfaces and no-op classes, I just used reflection. Thoughts?
There was a problem hiding this comment.
Cannot say I'm a fan especially of the calling side 😅
val distribution = Sentry.distribution() as? Distribution
If it's not a big lift to add the boilerplate, would you mind doing that (or asking claude lol)? I think it'd be great to keep it consistent with the other sub-APIs. You could retrieve the DistriubtionIntegration from getOptions.getIntegrations() or have an explicit distributionController like I did for replay.
There was a problem hiding this comment.
Sounds good, I can do that in a follow up
Added the missing isDistributionAvailable parameter (set to false) to installDefaultIntegrations method calls in test files to fix compilation errors. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Update sentry.api to include the new distribution() method signature to fix apiCheck failure. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ionIntegration Added consumer ProGuard rule in sentry-android-core to handle missing DistributionIntegration class when the distribution module is not included. This follows the same pattern used for other optional integrations like Replay and Timber.
runningcode
force-pushed
the
no/distribution-module-foundation
branch
from
f5e46f7 to
5dce724
Compare
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
I think this is not needed anymore if we define a namespace in the gradle config
| public var orgAuthToken: String = "" | ||
|
|
||
| /** Sentry organization slug */ | ||
| public var organizationSlug: String = "" |
There was a problem hiding this comment.
just a nit, but would be nice to be consistent with naming - i.e. either call this orgSlug or change the above to organizationAuthToken
There was a problem hiding this comment.
Sounds good. I’ll do this in a follow up
There was a problem hiding this comment.
since it is part of the public API, should we also use regular class here?
There was a problem hiding this comment.
sounds good, will do in a follow up
| public object UpToDate : UpdateStatus() | ||
|
|
||
| /** A new release is available for download. */ | ||
| public data class NewRelease(val info: UpdateInfo) : UpdateStatus() |
There was a problem hiding this comment.
same here about using regular classes
| @@ -0,0 +1,18 @@ | |||
| package io.sentry.android.distribution.internal | |||
There was a problem hiding this comment.
I'm thinking if we should move this out of the internal package? Because this will be the entry point for this submodule/integration and be accessed publicly from within sentry-android-core
There was a problem hiding this comment.
oh, hmm i messed this up then, I think I should have made the Distribution class the entry point. thanks for catching this!
Address PR feedback from #4712: - Refactor Sentry.distribution() to follow existing replay() pattern - Create IDistributionApi interface with core methods (checkForUpdateBlocking, checkForUpdate, downloadUpdate) - Add distributionController to SentryOptions with NoOp default - Use getCurrentScopes().getScope().getOptions().getDistributionController() access pattern - Remove reflection-based implementation for type safety - Provide NoOpDistributionApi fallback when module not available This follows the established architecture pattern used by replay API, allowing distribution integrations to register real implementations via options.setDistributionController() during integration registration. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Address PR feedback from #4712: - Refactor Sentry.distribution() to follow existing replay() pattern - Create IDistributionApi interface with core methods (checkForUpdateBlocking, checkForUpdate, downloadUpdate) - Add distributionController to SentryOptions with NoOp default - Use getCurrentScopes().getScope().getOptions().getDistributionController() access pattern - Remove reflection-based implementation for type safety - Provide NoOpDistributionApi fallback when module not available This follows the established architecture pattern used by replay API, allowing distribution integrations to register real implementations via options.setDistributionController() during integration registration. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…4731) * refactor(distribution): Use interface pattern for distribution API Address PR feedback from #4712: - Refactor Sentry.distribution() to follow existing replay() pattern - Create IDistributionApi interface with core methods (checkForUpdateBlocking, checkForUpdate, downloadUpdate) - Add distributionController to SentryOptions with NoOp default - Use getCurrentScopes().getScope().getOptions().getDistributionController() access pattern - Remove reflection-based implementation for type safety - Provide NoOpDistributionApi fallback when module not available This follows the established architecture pattern used by replay API, allowing distribution integrations to register real implementations via options.setDistributionController() during integration registration. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * feat(distribution): Complete distribution module implementation - Update AndroidOptionsInitializer to use new DistributionIntegration constructor - Refactor DistributionIntegration to implement IDistributionApi methods directly - Remove internal package structure and DistributionInternal dependency - Update class names from Distribution to DistributionIntegration for clarity - Convert data classes to regular classes to match API requirements - Rename organizationSlug to orgSlug for consistency - Implement downloadUpdate using Android Intent system - Remove completed Distribution singleton approach This completes the distribution module implementation to work with the new interface-based API pattern from the previous commit. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * feat(distribution): Move UpdateStatus and UpdateInfo to core sentry module - Move UpdateStatus and UpdateInfo from distribution module to core sentry module - Update IDistributionApi to use proper types instead of Object - Add UpdateCallback interface for type-safe async callbacks - Rename UpdateStatus.Error to UpdateError to avoid java.lang.Error clash - Update DistributionIntegration to implement IDistributionApi with proper types - Remove duplicate classes from distribution module - Regenerate API files with proper type signatures This provides full type safety for the distribution API while keeping the types accessible to all modules that might need them. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * fix(distribution): Update ProGuard rules for new DistributionIntegration class path - Update ProGuard rules from `internal.DistributionIntegration` to `DistributionIntegration` - Fixes R8 missing class error in release builds - Ensures DistributionIntegration class is properly kept during code shrinking This addresses the build failure in Android integration tests where R8 was removing the DistributionIntegration class that is referenced by AndroidOptionsInitializer. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * chore(distribution): Update API dump to reflect interface refactor - Update distribution module API file after refactoring to use interface pattern - Class renamed from Distribution to DistributionIntegration - Now implements IDistributionApi interface with proper method signatures - Removed UpdateInfo and UpdateStatus classes (moved to core sentry module) - Constructor now takes Context parameter - Method signatures use proper types instead of Object parameters 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * refactor(distribution): Move DistributionOptions to SentryOptions to resolve circular dependency - Move DistributionOptions from SentryAndroidOptions to SentryOptions to resolve circular dependency between sentry-android-core and sentry-android-distribution modules - Simplify DistributionIntegration.register() to work with SentryOptions directly instead of requiring SentryAndroidOptions - Remove separate DistributionOptions.kt file - Update API dumps 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * fix(distribution): Correct DistributionIntegration class name reference in SentryAndroid --------- Co-authored-by: Claude <[email protected]>
Summary
Adds Sentry Android Distribution module with automatic integration and
Sentry.distribution()API.Key Changes
Sentry.distribution()methodUsage
#skip-changelog
🤖 Generated with Claude Code